Drop .read/.aread from SyncByteStream/AsyncByteStream
#2407
Conversation
| assert isinstance(stream, httpx.AsyncByteStream) | ||
|
|
||
| sync_content = stream.read() | ||
| async_content = await stream.aread() |
There was a problem hiding this comment.
We're matching the style of the other test cases now.
These .read()/.aread() operations were only added to this test case so that we'd have coverage of those code lines.
| self._is_stream_consumed = True | ||
| if hasattr(self._stream, "read") and not isinstance( | ||
| self._stream, SyncByteStream | ||
| ): |
There was a problem hiding this comment.
The and not isinstance(self._stream, SyncByteStream) is what prompted me to clean these up.
It's such an oddly confusing branch - I couldn't figure out why it existed.
"Treat file-like objects as file-like objects, except for this one specific case".
No thanks.
| status_code, headers, stream, extensions = transport.handle_request(...) | ||
| try: | ||
| ... | ||
| finally: | ||
| stream.close() |
There was a problem hiding this comment.
This is an example from the old-style Transport API.
We had .read() so that users could eg. stream.read() here.
The Transport API now just returns fully fledged responses, so this is redundant.
|
Thanks for the review @michaeloliverx. |
|
...How should we handle that generally? |
I think this is okay, maybe if you are personally reviewing something you can go ahead and merge it yourself. |
We have a leftover bit of API on
SyncByteStream/AsyncByteStreamthat is a hangover from when we were designing the Transport API.They both have a convenience method for reading the stream, which was added to make testing at the low-level of the Transport API easier. However the methods are a bit kludgy because they don't actually fit the
read(max_bytes)that Python file interfaces use.I think we can just hard-drop these. They're undocumented, and you don't ever need them. The convenience methods are on the
Responseclass.